Skip to content

Conversation

warengonzaga
Copy link
Member

@warengonzaga warengonzaga commented Sep 3, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a resilient 3-layer storage (in-memory L1, Redis L2, PostgreSQL L3) and a BotsStore/UnifiedStorage layer.
    • Switched webhooks to queued, retryable processing with endpoints: /webhook/health, /webhook/metrics, /webhook/retry.
    • Added webhook queue processor and expanded health/metrics surface.
  • Documentation

    • Major README overhaul: architecture, Docker local dev, setup, deployment, monitoring, and health examples.
  • Chores

    • Updated .env.example (new DB/Redis URLs, PORT, timeout), docker-compose, and dependencies for queue/storage.

@warengonzaga warengonzaga self-assigned this Sep 3, 2025
@Copilot Copilot AI review requested due to automatic review settings September 3, 2025 14:02
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This is a major architectural overhaul upgrading the Discord bot from a simple Redis cache to a sophisticated 3-layer storage system with queue-based webhook processing, marking the v1.0.0 release.

  • Migration from Redis-only caching to a 3-layer architecture (Memory L1, Redis L2, PostgreSQL L3)
  • Implementation of queue-based webhook processing with BullMQ for improved reliability and scalability
  • Complete replacement of legacy cache utilities with the new BotsStore SDK

Reviewed Changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/utils/threadUtils.ts Updated thread utilities to use BotsStore 3-layer architecture with enhanced retry logic
src/utils/memory.ts Removed legacy memory cache module, replaced by UnifiedStorage
src/utils/database.ts Removed legacy Redis database module, replaced by BotsStore
src/utils/customerUtils.ts Migrated customer operations to use BotsStore with enhanced data models
src/utils/cache.ts Removed legacy Cacheable cache module
src/types/discord.ts Updated environment configuration to support new storage architecture
src/services/webhook.ts Complete rewrite to use queue-based processing with comprehensive monitoring
src/services/unthread.ts Updated to use BotsStore instead of legacy cache functions
src/services/QueueProcessor.ts New queue processing system with BullMQ, retry logic, and metrics
src/sdk/bots-brain/UnifiedStorage.ts New 3-layer storage engine with automatic fallback capabilities
src/sdk/bots-brain/BotsStore.ts New Discord-specific storage operations built on UnifiedStorage
src/index.ts Updated application initialization to use new storage architecture
src/events/threadCreate.ts Minor updates for customer email generation
src/events/messageDelete.ts Updated to use BotsStore for caching deleted messages
src/events/interactionCreate.ts Updated customer operations to use BotsStore
src/database/schema.sql New PostgreSQL schema for 3-layer architecture
package.json Added new dependencies for PostgreSQL, Redis, and queue processing
docker-compose.yml New Docker configuration for complete development environment
README.md Updated documentation for new architecture and features
.env.example Updated environment variables for new storage configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

coderabbitai bot commented Sep 3, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

At a high level: replaced the single-Redis/cache model with a 3-layer storage stack (L1 in-memory, L2 Redis, L3 PostgreSQL), introduced UnifiedStorage and a BotsStore facade, moved webhook processing to BullMQ-backed queues with a QueueProcessor/DLQ, added DB schema and Docker Compose, updated env/config, and refactored event handlers and utilities to use the new storage and queue primitives.

Changes

Cohort / File(s) Summary of Changes
Environment & Docs
./.env.example, ./README.md, ./docker-compose.yml, ./package.json
Env vars updated (removed REDIS_URL; added POSTGRES_URL, PLATFORM_REDIS_URL, WEBHOOK_REDIS_URL, PORT, UNTHREAD_HTTP_TIMEOUT_MS). README rewritten for 3-layer architecture, dev/deploy, observability. Added docker-compose for Postgres, two Redis services and bot. Added deps: bullmq, ioredis, pg, redis (+ typings).
Storage Core & DB Schema
src/sdk/bots-brain/UnifiedStorage.ts, src/sdk/bots-brain/BotsStore.ts, src/database/schema.sql
New UnifiedStorage (L1 memory, L2 Redis, L3 Postgres) with get/set/delete/health/metrics and cache-warming; BotsStore singleton built on UnifiedStorage + Postgres pool for customers, mappings, bot config, metrics, cleanup. Added full Postgres init schema, indexes, triggers and cleanup function.
Server, Types & Startup
src/index.ts, src/types/discord.ts
Startup now initializes/health-checks storage layers and webhook queue; enhanced /health and root endpoints; BotConfig requires POSTGRES_URL, PLATFORM_REDIS_URL, WEBHOOK_REDIS_URL (REDIS_URL made optional). New BotsStore/queue wiring and exported webhook service functions.
Webhook Queueing & Processing
src/services/webhook.ts, src/services/QueueProcessor.ts
Webhook handler now verifies HMAC and enqueues events to BullMQ (priority queues, DLQ); added endpoints /webhook/health, /webhook/metrics, /webhook/retry; new QueueProcessor manages queues, workers, retries, DLQ, metrics and health.
Service & Logic Refactors
src/services/unthread.ts, src/services/QueueProcessor.ts
Replaced memory/Keyv patterns with BotsStore calls for thread↔ticket mappings and deleted-message dedupe; updated function signatures to return ExtendedThreadTicketMapping and added error handling and retry option objects.
Event Handlers
src/events/interactionCreate.ts, src/events/messageDelete.ts, src/events/threadCreate.ts
Event handlers migrated from local memory/keyv to BotsStore APIs (customer storage, deletion cache, mapping cleanup); added email normalization/fallback generation, permission checks, longer starter-message retries, and TTL semantics adjusted to seconds.
Utilities Removed / Reworked
src/utils/cache.ts, src/utils/database.ts, src/utils/memory.ts, src/utils/customerUtils.ts, src/utils/threadUtils.ts
Removed legacy cache/database/memory modules (Keyv/Redis-backed cache removed). customerUtils now delegates to BotsStore and re-exports Customer type; threadUtils updated to use BotsStore, new fetchStarterMessage and enhanced retry/find signatures; some local helper types removed or re-exported for compatibility.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120+ minutes

Possibly related PRs

  • 🚀 release: v1.0.0-rc1 #45 — Prior work that introduced Redis-centric caching and webhook startup patterns; overlaps heavily with storage/startup/webhook surface changed here.
  • 🚀 release: v0.2.0-beta.6.5 #40 — Prior edits touching src/services/unthread (retry/logging/lookup behavior); related to the mapping and retry refactors in this PR.

Pre-merge checks (1 passed, 1 warning, 1 inconclusive)

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description is just “@coderabbitai summary” and does not provide any context or details about the changes in the PR, leaving reviewers without guidance on its purpose or scope. Please replace the placeholder with a concise summary of the PR’s objectives, highlighting the major changes such as the 3-layer storage architecture, webhook queue refactor, and updated Docker and README enhancements.
Title Check ❓ Inconclusive Title “🚀 release: v1.0.0” is generic and doesn’t convey the primary changes introduced by the PR; it indicates a version bump but fails to summarize the central feature additions like the 3-layer storage architecture and async webhook processing. Consider updating the title to succinctly reflect the main change, for example “Introduce 3-layer storage architecture and async webhook queue for v1.0.0 release” so that the key enhancements are clear at a glance.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 92.31% which is sufficient. The required threshold is 80.00%.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 15

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
package.json (1)

28-31: Match Node runtime and types

Engines target Node 22; types target Node 18. Update types to avoid mismatches.

-    "@types/node": "^18.19.0",
+    "@types/node": "^22.7.5",

Also applies to: 48-48

src/types/discord.ts (1)

69-70: Expose UNTHREAD_HTTP_TIMEOUT_MS in BotConfig

It’s in .env.example but missing here. Add for type-safety; parse to number at use-site.

  /** Port for webhook server (optional, defaults to 3000) */
  PORT?: string;
+ /** HTTP timeout for Unthread calls in ms (optional) */
+ UNTHREAD_HTTP_TIMEOUT_MS?: string;
README.md (1)

460-465: Align env var naming in README
Replace the outdated REDIS_URL reference with the two specific env vars:

- - Verify your `REDIS_URL` is correctly formatted
+ - Verify your `PLATFORM_REDIS_URL` and `WEBHOOK_REDIS_URL` are correctly formatted
src/utils/customerUtils.ts (1)

39-63: Harden external call: add timeout, JSON-parse safety, and richer error context

Network calls must be bounded and fail loudly with actionable detail.

- // Construct the API request to create a customer in Unthread
- const response = await fetch('https://api.unthread.io/api/customers', {
-   method: 'POST',
-   headers: {
-     'Content-Type': 'application/json',
-     'X-API-KEY': apiKey,
-   },
-   body: JSON.stringify({ name: user.username }),
- });
-
- if (!response.ok) {
-   throw new Error(`Failed to create customer: ${response.status}`);
- }
-
- const data = await response.json();
+ // Construct the API request to create a customer in Unthread (with timeout)
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ let data: any;
+ try {
+   const response = await fetch('https://api.unthread.io/api/customers', {
+     method: 'POST',
+     headers: {
+       'Content-Type': 'application/json',
+       'Accept': 'application/json',
+       'X-API-KEY': apiKey,
+     },
+     body: JSON.stringify({ name: user.username }),
+     signal: controller.signal,
+   });
+   if (!response.ok) {
+     const text = await response.text().catch(() => '');
+     throw new Error(`Failed to create customer: ${response.status} ${response.statusText}${text ? ` - ${text}` : ''}`);
+   }
+   data = await response.json().catch(() => ({}));
+ } catch (err) {
+   const reason = err instanceof Error ? err.message : 'Unknown error';
+   throw new Error(`Unthread customer creation failed: ${reason}`);
+ } finally {
+   clearTimeout(timeout);
+ }
🧹 Nitpick comments (32)
package.json (1)

34-34: Move type-only packages to devDependencies

@types/ioredis is type-only; keep runtime deps lean.

 "dependencies": {
-  "@types/ioredis": "^5.0.0",
   ...
 },
 "devDependencies": {
+  "@types/ioredis": "^5.0.0",
   "@types/pg": "^8.15.5",
   ...
 }

Also applies to: 49-49

src/types/discord.ts (1)

88-100: Tighten CustomerOperations types (optional)

unknown hampers consumers. If a Customer type exists (BotsStore), use it here for coherence.

-export interface CustomerOperations {
+export interface CustomerOperations<Customer = unknown, DiscordUser = unknown> {
   /** Create or retrieve existing customer */
-  getOrCreateCustomer: (user: unknown, email?: string) => Promise<unknown>;
+  getOrCreateCustomer: (user: DiscordUser, email?: string) => Promise<Customer>;
   /** Find customer by Discord user ID */
-  getCustomerByDiscordId: (discordId: string) => Promise<unknown>;
+  getCustomerByDiscordId: (discordId: string) => Promise<Customer | null>;
   /** Update existing customer record */
-  updateCustomer: (customer: unknown) => Promise<unknown>;
+  updateCustomer: (customer: Customer) => Promise<Customer>;
 }
.env.example (2)

1-9: Tidy ordering for dotenv linters

Place CLIENT_ID before DISCORD_BOT_TOKEN to quiet linters.

-# Discord Bot Configuration
-DISCORD_BOT_TOKEN=your_discord_bot_token_here
-CLIENT_ID=your_client_id_here
+# Discord Bot Configuration
+CLIENT_ID=your_client_id_here
+DISCORD_BOT_TOKEN=your_discord_bot_token_here
 GUILD_ID=your_guild_id_here

21-25: Keep optional keys ordered and end with newline

Move DEBUG_MODE before FORUM_CHANNEL_IDS and add a trailing newline.

-# Optional Configuration
-FORUM_CHANNEL_IDS=channel_id_1,channel_id_2,channel_id_3
-DEBUG_MODE=false
-PORT=3000
+# Optional Configuration
+DEBUG_MODE=false
+FORUM_CHANNEL_IDS=channel_id_1,channel_id_2,channel_id_3
+PORT=3000
+
docker-compose.yml (3)

69-79: Trim trailing spaces and normalize whitespace

Minor YAML lint; remove trailing spaces at noted lines.


53-91: Add a healthcheck for the bot container

Improves orchestration and readiness signaling.

   discord-bot:
     build: .
     container_name: unthread-discord-bot
@@
     command: yarn dev
     restart: unless-stopped
+    healthcheck:
+      test: ["CMD", "wget", "-qO-", "http://localhost:${PORT:-3000}/health"]
+      interval: 15s
+      timeout: 5s
+      retries: 5

75-79: Avoid embedding credentials in DSNs for production

The in-container POSTGRES_URL includes user/pass. Fine for local, but use secrets or discrete envs for prod.

Example:

  • Inject POSTGRES_USER/POSTGRES_PASSWORD via secrets.
  • Build DSN at runtime from discrete vars.
README.md (3)

94-107: Convert bold lines to proper headings (fixes MD036).

Avoid using emphasis as headings. Promote the L1/L2/L3 labels to H4 for cleaner structure and to satisfy markdownlint.

-**Layer 1 (L1): In-Memory Cache**
+#### Layer 1 (L1): In-Memory Cache
@@
-**Layer 2 (L2): Redis Cache**
+#### Layer 2 (L2): Redis Cache
@@
-**Layer 3 (L3): PostgreSQL Database**
+#### Layer 3 (L3): PostgreSQL Database

262-281: Normalize list indentation (fixes MD007).

Unordered list items are indented by 3 spaces. Align bullets to column 0 under each section header.

-   - `DISCORD_BOT_TOKEN`: The token you copied from the "Bot" tab.
-   - `CLIENT_ID`: Your application's client ID, found in the "General Information" tab.
-   - `GUILD_ID`: The ID of the Discord server where you want to deploy the bot. [How to Get Your Discord Server ID](#how-to-get-your-discord-server-id)
+ - `DISCORD_BOT_TOKEN`: The token you copied from the "Bot" tab.
+ - `CLIENT_ID`: Your application's client ID, found in the "General Information" tab.
+ - `GUILD_ID`: The ID of the Discord server where you want to deploy the bot. [How to Get Your Discord Server ID](#how-to-get-your-discord-server-id)
@@
-   - `UNTHREAD_API_KEY`: Your Unthread API key.
-   - `UNTHREAD_SLACK_CHANNEL_ID`: Your Unthread Slack channel ID for ticket routing.
-   - `UNTHREAD_WEBHOOK_SECRET`: Your Unthread webhook secret.
+ - `UNTHREAD_API_KEY`: Your Unthread API key.
+ - `UNTHREAD_SLACK_CHANNEL_ID`: Your Unthread Slack channel ID for ticket routing.
+ - `UNTHREAD_WEBHOOK_SECRET`: Your Unthread webhook secret.
@@
-   - `POSTGRES_URL`: PostgreSQL connection string (e.g., `postgres://user:password@localhost:5432/database`)
-   - `PLATFORM_REDIS_URL`: Redis cache connection URL (e.g., `redis://localhost:6379`)
-   - `WEBHOOK_REDIS_URL`: Redis queue connection URL (e.g., `redis://localhost:6380`)
+ - `POSTGRES_URL`: PostgreSQL connection string (e.g., `postgres://user:password@localhost:5432/database`)
+ - `PLATFORM_REDIS_URL`: Redis cache connection URL (e.g., `redis://localhost:6379`)
+ - `WEBHOOK_REDIS_URL`: Redis queue connection URL (e.g., `redis://localhost:6380`)
@@
-   - `FORUM_CHANNEL_IDS`: Comma-separated list of forum channel IDs for automatic ticket creation.
-   - `DEBUG_MODE`: Set to `true` for verbose logging during development (default: `false`).
-   - `PORT`: Port for the webhook server (default: `3000`).
+ - `FORUM_CHANNEL_IDS`: Comma-separated list of forum channel IDs for automatic ticket creation.
+ - `DEBUG_MODE`: Set to `true` for verbose logging during development (default: `false`).
+ - `PORT`: Port for the webhook server (default: `3000`).

156-167: Modernize Docker command usage (optional).

Prefer docker compose over the legacy docker-compose binary for current Docker versions.

-docker-compose up -d
+docker compose up -d
@@
-docker-compose logs -f discord-bot
+docker compose logs -f discord-bot
@@
-docker-compose down
+docker compose down
@@
-docker-compose down -v
+docker compose down -v
src/sdk/bots-brain/UnifiedStorage.ts (3)

181-196: Redis set: safeguard JSON serialization failures.

If value contains circular refs, JSON.stringify throws and aborts the write-through. Catch and degrade gracefully.

-    try {
-        const serialized = JSON.stringify(value);
+    try {
+        const serialized = JSON.stringify(value, (_k, v) => (typeof v === 'bigint' ? v.toString() : v));
         if (ttlSeconds) {
             await this.client.setEx(key, ttlSeconds, serialized);
         } else {
             await this.client.set(key, serialized);
         }
     }

411-422: Write-through strategy: prefer L3-first and tolerate cache failures.

A single cache failure will reject Promise.all and bubble up even if Postgres succeeds. Write source-of-truth first, then update caches with allSettled.

-    // Write to all layers simultaneously
-    await Promise.all([
-        this.l1Memory.set(key, value, ttl),
-        this.l2Redis.set(key, value, ttl),
-        this.l3Postgres.set(key, value, ttl),
-    ]);
+    // Write source-of-truth first
+    await this.l3Postgres.set(key, value, ttl);
+    // Then update caches, tolerate failures
+    await Promise.allSettled([
+        this.l1Memory.set(key, value, ttl),
+        this.l2Redis.set(key, value, ttl),
+    ]);

147-159: Use ready/end events for accurate Redis readiness.

connect fires before the client is fully ready. Switch to ready and track end to avoid false positives.

-            this.client.on('connect', () => {
+            this.client.on('ready', () => {
                 this.connected = true;
-                LogEngine.info('Redis L2 cache connected successfully');
+                LogEngine.info('Redis L2 cache ready');
             });
+            this.client.on('end', () => { this.connected = false; });
src/database/schema.sql (1)

54-61: Consider relational integrity on customer_id and status domain.

Optional but valuable: add FK to customers(discord_id) and constrain status.

-    customer_id VARCHAR(255),  -- reference to customers table
-    status VARCHAR(20) DEFAULT 'active',  -- active, closed, archived
+    customer_id VARCHAR(255) REFERENCES customers(discord_id),
+    status VARCHAR(20) DEFAULT 'active' CHECK (status IN ('active','closed','archived')),
src/events/interactionCreate.ts (1)

188-191: Mapping cleanup works; consider a helper to reduce duplicate deletes.

clearCache('mapping', threadId) and again with ticketId results in four delete attempts (two will be no-ops). Not harmful, but a small helper like clearThreadTicketMapping(threadId, ticketId) would be cleaner.

src/events/messageDelete.ts (1)

33-34: Tighten types for channel deletion history.

Use a concrete interface to avoid unchecked casts and Record<string, unknown>.

-const recentlyDeletedInChannel = (await botsStore.getBotConfig<Array<Record<string, unknown>>>(channelKey)) || [];
+interface DeletedEntry { messageId: string; timestamp: number }
+const recentlyDeletedInChannel =
+  (await botsStore.getBotConfig<DeletedEntry[]>(channelKey)) || [];
...
-const filteredList = recentlyDeletedInChannel
-  .filter((item: Record<string, unknown>) => (item.timestamp as number) > oneMinuteAgo)
+const filteredList = recentlyDeletedInChannel
+  .filter((item) => item.timestamp > oneMinuteAgo)
   .slice(-10);

Also applies to: 45-49

src/services/webhook.ts (1)

101-101: ESLint: remove trailing spaces.

Two lines flagged by no-trailing-spaces. Quick tidy below.

-		// Use constant-time comparison to prevent timing attacks
+		// Use constant-time comparison to prevent timing attacks
@@
-		// Prevent crash by checking buffer lengths match
+		// Prevent crash by checking buffer lengths match

Also applies to: 106-106

src/utils/customerUtils.ts (2)

12-18: Use type-only import for Customer and keep the re-export

Avoids an unnecessary runtime import while preserving the public surface.

-import { BotsStore, Customer } from '../sdk/bots-brain/BotsStore';
+import { BotsStore } from '../sdk/bots-brain/BotsStore';
+import type { Customer } from '../sdk/bots-brain/BotsStore';
 // Re-export Customer interface for backward compatibility
 export { Customer } from '../sdk/bots-brain/BotsStore';

94-113: Mitigate duplicate Unthread customers under races

Two concurrent cache misses can both create upstream customers. Add a quick second-chance lookup before the create.

-    // Create new customer in Unthread if not found
-    LogEngine.info(`Creating new customer for Discord user ${user.id}`);
+    // Create new customer in Unthread if not found
+    // Second-chance lookup to reduce race-triggered duplicates
+    customer = await botsStore.getCustomerByDiscordId(user.id);
+    if (customer) {
+      LogEngine.debug(`Customer appeared after recheck for Discord user ${user.id}`);
+      return customer;
+    }
+    LogEngine.info(`Creating new customer for Discord user ${user.id}`);
src/utils/threadUtils.ts (4)

44-48: Preserve instanceof semantics for custom Error across ESM/CJS

Ensures retries trigger correctly even across module boundaries.

 export class MappingNotFoundError extends Error {
   constructor(message: string) {
     super(message);
     this.name = 'MappingNotFoundError';
+    // Ensure proper prototype chain for instanceof checks
+    Object.setPrototypeOf(this, new.target.prototype);
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, MappingNotFoundError);
+    }
   }
 }

81-115: Remove duplicate docblock

Two consecutive JSDoc blocks describe the same function; keep the second (more detailed) block to avoid drift.

-/**
- * Fetches a Discord thread using an Unthread ticket ID with retry logic for race conditions
- *
- * This function extends findDiscordThreadByTicketId with intelligent retry logic to handle
- * edge cases where webhook events arrive before ticket mappings are fully propagated in storage.
- *
- * Common scenarios this handles:
- * - Storage propagation delays under high load
- * - Network hiccups during mapping creation
- * - Temporary storage system unavailability
- * - Webhooks arriving faster than expected from Unthread
- *
- * @param unthreadTicketId - Unthread ticket/conversation ID
- * @param options - Retry configuration options
- * @param lookupFunction - Optional function to lookup ticket mapping by Unthread ID
- * @returns Object containing mapping and thread
- * @throws {MappingNotFoundError} When ticket mapping not found after all retries
- * @throws {Error} When Discord API errors occur or thread is not accessible
- * @throws {Error} When lookup function fails for non-mapping reasons
- *
- * @example
- * ```typescript
- * try {
- *   const result = await findDiscordThreadByTicketIdWithRetry(
- *     'ticket123',
- *     { maxAttempts: 5, maxRetryWindow: 15000 }
- *   );
- *   console.log(`Found thread: ${result.discordThread.id}`);
- * } catch (error) {
- *   if (error instanceof MappingNotFoundError) {
- *     console.log("Ticket mapping not found - likely external ticket");
- *   }
- * }
- * ```
- */

170-181: Avoid double lookup when a custom mapping is already found

If lookupFunction returns a mapping, reuse it to fetch the thread directly rather than performing another store lookup.

-      if (lookupFunction) {
-        const customMapping = await lookupFunction(unthreadTicketId);
-        if (customMapping) {
-          // If custom lookup found a mapping, use the standard resolution
-          return await findDiscordThreadByTicketId(unthreadTicketId);
-        }
-      }
+      if (lookupFunction) {
+        const customMapping = await lookupFunction(unthreadTicketId);
+        if (customMapping) {
+          return await (async () => {
+            const discordClient = (global as typeof globalThis).discordClient;
+            if (!discordClient) throw new Error('Discord client is not initialized or unavailable.');
+            const channel = await discordClient.channels.fetch(customMapping.discordThreadId);
+            if (!channel) throw new Error(`Discord thread with ID ${customMapping.discordThreadId} not found.`);
+            if (!channel.isThread()) throw new Error(`Discord channel with ID ${customMapping.discordThreadId} is not a thread.`);
+            return { ticketMapping: { status: 'active', ...customMapping }, discordThread: channel as ThreadChannel };
+          })();
+        }
+      }

Note: If you prefer not to inline this logic, extract it into a small helper (e.g., resolveMappingToThread) within this module.


178-178: Lint: remove trailing spaces

ESLint flagged trailing whitespace. Please run yarn lint --fix or trim manually.

src/index.ts (2)

84-92: Include database_pool in startup health gate

BotsStore.healthCheck returns database_pool; failing it at startup avoids late surprises.

- if (!health.memory) failedLayers.push('memory');
- if (!health.redis) failedLayers.push('redis');
- if (!health.postgres) failedLayers.push('postgres');
+ if (!health.memory) failedLayers.push('memory');
+ if (!health.redis) failedLayers.push('redis');
+ if (!health.postgres) failedLayers.push('postgres');
+ if (health.database_pool === false) failedLayers.push('database_pool');

114-143: Normalize health catch-all shape

Return database_pool: false in the error path for consistent clients.

-      layers: { memory: false, redis: false, postgres: false },
+      layers: { memory: false, redis: false, postgres: false, database_pool: false },
src/sdk/bots-brain/BotsStore.ts (2)

506-524: Cache clearing API can target wrong keys.

pattern: 'customer' with a single identifier blasts both customer:discord:{id} and customer:unthread:{id} even though only one is valid. Split identifier by type to avoid ineffective deletes.

- async clearCache(pattern: 'customer' | 'mapping' | 'config', identifier?: string): Promise<void> {
+ async clearCache(
+   pattern: 'customer' | 'mapping' | 'config',
+   identifier?: string,
+   idType?: 'discord' | 'unthread',
+ ): Promise<void> {
@@
-  const patterns = {
-    customer: identifier ? [`customer:discord:${identifier}`, `customer:unthread:${identifier}`] : [],
+  const patterns = {
+    customer: identifier
+      ? idType === 'unthread'
+        ? [`customer:unthread:${identifier}`]
+        : [`customer:discord:${identifier}`]
+      : [],

131-136: Minor: simplify env lookup.

process.env[name as keyof NodeJS.ProcessEnv] is unnecessary and obscures intent. if (!process.env[name]) is sufficient.

- if (!process.env[name as keyof NodeJS.ProcessEnv]) {
+ if (!process.env[name]) {
src/services/unthread.ts (3)

188-205: Remove outdated/duplicate doc block.

Two adjacent docs for the same function; the first references legacy cache. Keep the BotsStore version to avoid confusion.

-/**
- * Binds a Discord thread to an Unthread ticket
- *
- * Creates a bidirectional mapping in the cache to enable message forwarding
- * and webhook event routing between the two systems.
- *
- * @param unthreadTicketId - Unthread ticket ID
- * @param discordThreadId - Discord thread ID
- * @throws {Error} When cache operations fail
- */

116-118: Avoid logging PII in debug.

Email appears in logs. Consider redacting or hashing in non-dev environments.

- LogEngine.debug(`Customer: ${customer?.unthreadCustomerId || 'unknown'} (${customer?.email || email})`);
+ const emailSafe = (process.env.DEBUG_MODE === 'true') ? (customer?.email || email) : '<redacted>';
+ LogEngine.debug(`Customer: ${customer?.unthreadCustomerId || 'unknown'} (${emailSafe})`);

423-430: Consider centralizing retry defaults.

retryOptions literals are duplicated across handlers. Extract to a constant or config for consistency.

src/services/QueueProcessor.ts (2)

275-283: Parity: track priority worker events too.

Metrics/logs hook only webhookWorker. Mirror handlers for priorityWorker.

   this.webhookWorker.on('completed', (job) => {
     LogEngine.debug(`Webhook processed successfully: ${job.data.eventType}`);
   });
   this.webhookWorker.on('failed', (job, err) => {
     LogEngine.error(`Webhook processing failed: ${job?.data.eventType} - ${err.message}`);
   });
+  this.priorityWorker.on('completed', (job) => {
+    LogEngine.debug(`Priority webhook processed successfully: ${job.data.eventType}`);
+  });
+  this.priorityWorker.on('failed', (job, err) => {
+    LogEngine.error(`Priority webhook processing failed: ${job?.data.eventType} - ${err.message}`);
+  });

301-303: Graceful Redis shutdown.

Use quit() (async) rather than disconnect() for graceful close.

- await this.redis.disconnect();
+ await this.redis.quit();
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e3e2d57 and 547cc72.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (20)
  • .env.example (1 hunks)
  • README.md (3 hunks)
  • docker-compose.yml (1 hunks)
  • package.json (1 hunks)
  • src/database/schema.sql (1 hunks)
  • src/events/interactionCreate.ts (3 hunks)
  • src/events/messageDelete.ts (3 hunks)
  • src/events/threadCreate.ts (2 hunks)
  • src/index.ts (6 hunks)
  • src/sdk/bots-brain/BotsStore.ts (1 hunks)
  • src/sdk/bots-brain/UnifiedStorage.ts (1 hunks)
  • src/services/QueueProcessor.ts (1 hunks)
  • src/services/unthread.ts (8 hunks)
  • src/services/webhook.ts (4 hunks)
  • src/types/discord.ts (1 hunks)
  • src/utils/cache.ts (0 hunks)
  • src/utils/customerUtils.ts (3 hunks)
  • src/utils/database.ts (0 hunks)
  • src/utils/memory.ts (0 hunks)
  • src/utils/threadUtils.ts (7 hunks)
💤 Files with no reviewable changes (3)
  • src/utils/cache.ts
  • src/utils/database.ts
  • src/utils/memory.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
📚 Learning: 2025-08-28T11:18:55.746Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository's package.json, the build and deployment scripts (start, deploycommand, build) consistently use yarn, while development scripts (dev, lint, lint:fix) invoke tools directly. This represents a clean separation between orchestration scripts and development tool invocation.

Applied to files:

  • README.md
📚 Learning: 2025-08-28T11:18:55.746Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.

Applied to files:

  • README.md
📚 Learning: 2025-08-30T09:26:39.331Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#45
File: src/services/unthread.ts:184-193
Timestamp: 2025-08-30T09:26:39.331Z
Learning: In the wgtechlabs/unthread-discord-bot repository, the current custom Unthread service implementation in src/services/unthread.ts is planned to be replaced with an official SDK in the future, as mentioned by warengonzaga.

Applied to files:

  • README.md
  • src/services/unthread.ts
  • src/utils/threadUtils.ts
🧬 Code graph analysis (9)
src/events/threadCreate.ts (1)
src/utils/customerUtils.ts (1)
  • getOrCreateCustomer (89-120)
src/events/messageDelete.ts (1)
src/sdk/bots-brain/BotsStore.ts (1)
  • BotsStore (78-573)
src/sdk/bots-brain/BotsStore.ts (2)
src/types/discord.ts (2)
  • ThreadTicketMapping (27-36)
  • BotConfig (43-70)
src/sdk/bots-brain/UnifiedStorage.ts (1)
  • UnifiedStorage (330-504)
src/sdk/bots-brain/UnifiedStorage.ts (1)
src/config/logger.ts (1)
  • LogEngine (35-35)
src/services/webhook.ts (2)
src/services/QueueProcessor.ts (1)
  • QueueProcessor (91-582)
src/config/logger.ts (1)
  • LogEngine (35-35)
src/index.ts (3)
src/sdk/bots-brain/BotsStore.ts (2)
  • BotsStore (78-573)
  • BotConfig (59-63)
src/services/webhook.ts (1)
  • initializeWebhookService (55-64)
src/types/discord.ts (1)
  • BotConfig (43-70)
src/services/unthread.ts (2)
src/sdk/bots-brain/BotsStore.ts (2)
  • BotsStore (78-573)
  • ExtendedThreadTicketMapping (48-54)
src/utils/threadUtils.ts (2)
  • findDiscordThreadByTicketIdWithRetry (149-240)
  • findDiscordThreadByTicketId (297-345)
src/utils/threadUtils.ts (2)
src/types/discord.ts (1)
  • ThreadTicketMapping (27-36)
src/sdk/bots-brain/BotsStore.ts (2)
  • ExtendedThreadTicketMapping (48-54)
  • BotsStore (78-573)
src/utils/customerUtils.ts (2)
src/sdk/bots-brain/BotsStore.ts (3)
  • Customer (33-43)
  • BotsStore (78-573)
  • getCustomerByDiscordId (251-287)
src/config/logger.ts (1)
  • LogEngine (35-35)
🪛 Checkov (3.2.334)
docker-compose.yml

[MEDIUM] 76-77: Basic Auth Credentials

(CKV_SECRET_4)

🪛 YAMLlint (1.37.1)
docker-compose.yml

[error] 69-69: trailing spaces

(trailing-spaces)


[error] 74-74: trailing spaces

(trailing-spaces)


[error] 79-79: trailing spaces

(trailing-spaces)


[error] 103-103: no new line character at the end of file

(new-line-at-end-of-file)

🪛 dotenv-linter (3.3.0)
.env.example

[warning] 3-3: [UnorderedKey] The CLIENT_ID key should go before the DISCORD_BOT_TOKEN key

(UnorderedKey)


[warning] 23-23: [UnorderedKey] The DEBUG_MODE key should go before the FORUM_CHANNEL_IDS key

(UnorderedKey)


[warning] 27-27: [EndingBlankLine] No blank line at the end of the file

(EndingBlankLine)

🪛 markdownlint-cli2 (0.17.2)
README.md

94-94: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


99-99: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


104-104: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


263-263: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


264-264: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


265-265: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


268-268: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


269-269: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


270-270: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


273-273: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


274-274: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


275-275: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


278-278: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


279-279: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)


280-280: Unordered list indentation
Expected: 0; Actual: 3

(MD007, ul-indent)

🪛 ESLint
src/services/webhook.ts

[error] 101-101: Trailing spaces not allowed.

(no-trailing-spaces)


[error] 106-106: Trailing spaces not allowed.

(no-trailing-spaces)

src/utils/threadUtils.ts

[error] 178-178: Trailing spaces not allowed.

(no-trailing-spaces)

🔇 Additional comments (15)
package.json (1)

7-14: Yarn PnP sanity check for dev workflow

With Yarn 4/PnP, nodemon + ts-node can be finicky. Ensure it resolves via PnP.

Run locally:

  • yarn dev
  • Touch a TS file; confirm live reload compiles without ESM loader errors.
    If issues arise, consider: nodemon --exec "yarn node --loader ts-node/esm" src/index.ts (if ESM) or use SDK shims. I can adjust scripts once you confirm the module mode.
src/types/discord.ts (1)

56-63: Config shape solid; add note on legacy and parsing

Optional REDIS_URL for legacy is fine; the three new URLs being required aligns with the 3-layer design.

src/database/schema.sql (1)

102-104: Single trigger suffices; ensure it remains after the function.

This one is correctly placed after the function. No change needed beyond removing the earlier duplicate.

src/events/interactionCreate.ts (1)

19-21: BotsStore initialization precedes all event handlersmain() calls await validateStartupRequirements() (which runs BotsStore.initialize()) before client.login(), so every BotsStore.getInstance() in your loaded events happens only after initialization.

src/events/messageDelete.ts (3)

25-29: TTL semantics look correct; nice switch to seconds.

Storing per-message metadata for 300s aligns with the comment. LGTM.


48-50: Channel cache TTL matches the one-minute window.

Good alignment between retention and TTL.


20-21: Initialization confirmed: BotsStore.initialize() is awaited at startup
initialize() runs in src/index.ts (lines 81–82) before any getInstance() calls, so event handlers (including messageDelete) use a valid instance.

src/services/webhook.ts (5)

229-268: Health endpoint: sensible status mapping.

Returns 200 for healthy/degraded and 503 for unhealthy. Looks good.


273-288: Metrics endpoint: straightforward and safe.

Defensive 503 when uninitialized and 500 on internal errors is appropriate.


293-314: Retry endpoint: guarded and parameterized.

limit handling and guarded initialization look solid.


49-64: initializeWebhookService is already called at startup
Invocation confirmed in src/index.ts:97; no further action required.


169-185: No changes needed—Unthread signs URL verification events
Affirmative, sir: Unthread applies HMAC-SHA256 signatures to all webhook deliveries, including url_verification, using a raw hex digest in the X-Unthread-Signature header. The current signature validation placement is appropriate.

src/utils/customerUtils.ts (1)

140-163: LGTM on lookup wrapper

Validation, logging, and error surfacing align with BotsStore semantics.

src/utils/threadUtils.ts (1)

333-345: LGTM on thread fetch path

Clear errors for missing client/channel and precise thread-type guard.

src/index.ts (1)

216-222: LGTM on startup logs

Helpful, precise operational breadcrumbs on boot.

@warengonzaga warengonzaga marked this pull request as draft September 5, 2025 16:21
…-8a29-0946a5311940

🔧 fix: resolve critical CodeRabbit review issues for type safety and retry logic
@warengonzaga
Copy link
Member Author

@CodeRabbit can you check if all of your previous reviews have been addressed?

Copy link

coderabbitai bot commented Sep 9, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll check all my previous reviews on this PR to verify if they've been addressed, sir. Let me gather that information for you.


🏁 Script executed:

#!/bin/bash
# Get all review comments made by coderabbitai on this PR
echo "Fetching all review comments by coderabbitai on PR #62..."
gh pr view 62 --json reviews,reviewDecision --repo wgtechlabs/unthread-discord-bot

echo -e "\n=== Getting detailed review comments ==="
gh api repos/wgtechlabs/unthread-discord-bot/pulls/62/reviews --paginate | jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, state: .state, submitted_at: .submitted_at, body: .body}'

echo -e "\n=== Getting review comments on code ==="
gh api repos/wgtechlabs/unthread-discord-bot/pulls/62/comments --paginate | jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .line, body: .body, created_at: .created_at}'

Length of output: 138584


@warengonzaga Excellent question, sir! Let me conduct a comprehensive analysis of my previous review from September 3rd to assess the current status of all identified issues.

Status Analysis: Previous Review Items

Based on my major review on September 3rd, 2025 which posted 15 actionable comments, here's the current status:

Resolved Issues (Confirmed via recent interactions):

  1. Schema TTL Issue - The storage_cache.expires_at NOT NULL constraint has been resolved
  2. Email Generation - Stable user ID-based email generation with .invalid domain has been implemented
  3. Webhook Management Endpoints - Simplified per your v1.0.0 preferences (retry endpoint removed, health/metrics made general-only)

🔄 Issues That May Still Need Attention:

Critical Security & Reliability:

  • Signature Verification: Raw body capture middleware for webhook HMAC validation
  • DB Connection Leaks: try/finally blocks for PostgreSQL client release in BotsStore
  • Queue Processing Logic: BullMQ retry mechanism (jobs marked complete instead of throwing errors)

Schema & Data Integrity:

  • PG Row Mapping: Snake_case to camelCase conversion in BotsStore (critical for functionality)
  • Date Type Consistency: Customer/ExtendedThreadTicketMapping timestamp standardization
  • Redundant Index: Unnecessary index on platform_message_id (unique constraint already provides it)

Code Quality & Performance:

  • UnifiedStorage Health Check: Promise.allSettled status checking (always returns healthy)
  • Write-Through Strategy: L3-first writes with cache failure tolerance
  • Type Safety: Various TypeScript improvements and error handling enhancements

Would you like me to prioritize and provide specific implementation guidance for any of these remaining items, or shall I generate a comprehensive status report with detailed findings for each category?

The architectural foundation you've built is impressive - we're now fine-tuning for production excellence! 🎯


🧠 Learnings used
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.767Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.

Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#62
File: src/index.ts:339-342
Timestamp: 2025-09-05T15:49:33.268Z
Learning: In the wgtechlabs/unthread-discord-bot repository, for the v1.0.0 release, the user prefers to simplify webhook management endpoints by removing the retry endpoint entirely and making /webhook/health and /webhook/metrics show only general, non-sensitive information rather than detailed operational data.

@warengonzaga warengonzaga marked this pull request as ready for review September 9, 2025 12:48
@warengonzaga warengonzaga marked this pull request as draft September 9, 2025 12:49
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/events/interactionCreate.ts (1)

84-91: Unify fallback email: use stable ID with .invalid TLD (and avoid usernames).

Align with threadCreate and prior guidance; prefer ${user.id}@discord.invalid and coalesce to existing customer email.

-	if (!email || email.trim() === '') {
-		// If no email provided, try to get existing customer record
-		const existingCustomer = await getCustomerByDiscordId(interaction.user.id);
-		email = existingCustomer?.email || `${interaction.user.username}@discord.user`;
-		LogEngine.debug(`Using fallback email for user ${interaction.user.id}: ${email}`);
-	}
+	if (!email || email.trim() === '') {
+		const existingCustomer = await getCustomerByDiscordId(interaction.user.id);
+		const fallbackEmail = `${interaction.user.id}@discord.invalid`;
+		email = existingCustomer?.email ?? fallbackEmail;
+		LogEngine.debug(`Using fallback email for user ${interaction.user.id}: ${email}`);
+	}
♻️ Duplicate comments (5)
src/services/QueueProcessor.ts (1)

151-154: Avoid unsafe env access cast; prefer direct check or a typed config.

This assertion as keyof NodeJS.ProcessEnv sidesteps type safety. Use a plain presence check or a typed schema (e.g., zod).

- if (!process.env[name as keyof NodeJS.ProcessEnv]) {
+ if (!process.env[name]) {
src/services/webhook.ts (1)

91-104: Header normalization looks good; prefix expectation matches computed value.

The array-safe header handling and ‘sha256=’ prefix alignment address prior feedback.

src/utils/threadUtils.ts (2)

149-154: Parameter order vs. docs/usage — settle on one and propagate.

Current signature is (id, options, lookupFunction). Previous guidance preferred (id, lookupFunction, options). Choose one and update callers/docs accordingly.

To inspect call sites and minimize churn:

#!/bin/bash
rg -nP --type=ts 'findDiscordThreadByTicketIdWithRetry\s*\(' -C2

171-177: Custom lookup result not utilized — defeats purpose under eventual consistency.

If lookupFunction returns a mapping, we re-query storage and risk missing it again. Use the provided mapping to fetch the thread immediately, and only augment mapping if needed.

 			if (lookupFunction) {
 				const customMapping = await lookupFunction(unthreadTicketId);
 				if (customMapping) {
-					// If custom lookup found a mapping, use the standard resolution
-					return await findDiscordThreadByTicketId(unthreadTicketId);
+					// Use the provided mapping directly to fetch the thread
+					const discordClient = (global as typeof globalThis).discordClient;
+					if (!discordClient) {
+						throw new Error('Discord client is not initialized or unavailable.');
+					}
+					const channel = await discordClient.channels.fetch(customMapping.discordThreadId);
+					if (!channel) {
+						throw new Error(`Discord thread with ID ${customMapping.discordThreadId} not found.`);
+					}
+					if (!channel.isThread()) {
+						throw new Error(`Discord channel with ID ${customMapping.discordThreadId} is not a thread.`);
+					}
+					// Attempt to enrich to ExtendedThreadTicketMapping; fall back to a cast if not found
+					const botsStore = BotsStore.getInstance();
+					const extended = await botsStore.getMappingByTicketId(unthreadTicketId);
+					return {
+						ticketMapping: (extended ?? (customMapping as unknown as ExtendedThreadTicketMapping)),
+						discordThread: channel,
+					};
 				}
 			}

If you prefer stricter typing, we can instead change lookupFunction to return ExtendedThreadTicketMapping.

src/index.ts (1)

158-206: Centralize env validation (repeat of prior note).

This file re-implements checks also present in BotsStore and services. Consider a shared config validator to DRY this up.

🧹 Nitpick comments (20)
src/services/QueueProcessor.ts (4)

11-26: Repair corrupted docblock; stray code leaked into header comment.

There’s executable-looking code embedded in the top JSDoc, which will confuse readers and linters. Please restore the bullet and remove the stray lines.

- * - Job priority and rate li		if (!ret			await this.sendToDLQ(job.data, errorMessage);
-			LogEngine.error(`Job moved to DLQ: ${eventType} - ${errorMessage}`);
-		}
-
-		// Always throw for BullMQ retry mechanism to work
-		// BullMQ will handle the retry logic based on job configuration
-		throw error;e || job.attemptsMade >= this.config.maxRetries) {
-			// Send to dead letter queue for non-retryable errors or max retries reached
-			await this.sendToDLQ(job.data, errorMessage);
-			LogEngine.error(`Job moved to DLQ: ${eventType} - ${errorMessage}`);
-		}
-
-		// Always throw for BullMQ retry mechanism to work
-		// BullMQ will handle the retry logic based on job configuration
-		throw error;
+ * - Job priority and rate limiting

259-288: Add QueueEvents for the priority queue or fold metrics into worker events.

Currently, QueueEvents only listens on webhook-events; priority completions/failures aren’t counted in metrics.

-	this.queueEvents = new QueueEvents('webhook-events', { connection: this.redis });
+	this.queueEvents = new QueueEvents('webhook-events', { connection: this.redis });
+	const priorityEvents = new QueueEvents('webhook-priority', { connection: this.redis });
@@
-	this.queueEvents.on('completed', ({ jobId }) => {
+	const onCompleted = ({ jobId }: { jobId: string }) => {
 		this.updateMetrics('completed');
 		LogEngine.debug(`Webhook job completed: ${jobId}`);
-	});
+	};
+	this.queueEvents.on('completed', onCompleted);
+	priorityEvents.on('completed', onCompleted);
@@
-	this.queueEvents.on('failed', ({ jobId, failedReason }) => {
+	const onFailed = ({ jobId, failedReason }: { jobId: string; failedReason: string }) => {
 		this.updateMetrics('failed');
 		LogEngine.error(`Webhook job failed: ${jobId} - ${failedReason}`);
-	});
+	};
+	this.queueEvents.on('failed', onFailed);
+	priorityEvents.on('failed', onFailed);

Remember to close priorityEvents during shutdown.


568-597: DLQ retry should read stored error from DLQJobData; job.failedReason will be empty.

DLQ jobs have no worker, so they sit in “waiting”. Read errorMessage from data and narrow the type.

-			// Query 'failed' and 'waiting' jobs instead of 'completed'
-			const failedJobs = await this.dlqQueue.getJobs(['failed', 'waiting'], 0, limit - 1);
+			// DLQ jobs are enqueued and left in 'waiting'
+			const failedJobs = await this.dlqQueue.getJobs(['waiting'], 0, limit - 1);
@@
-				const originalData = job.data as WebhookJobData;
+				const originalData = job.data as DLQJobData;
@@
-				const errorMessage = job.failedReason || 'Unknown error';
+				const errorMessage = originalData.errorMessage || 'Unknown error';

334-375: Minor duplication: rely on defaultJobOptions for attempts/backoff.

You already set attempts/backoff on the queue; passing them again per add is redundant unless you intend to override per-job.

src/events/interactionCreate.ts (2)

94-105: Do not invalidate caches right after store; it negates the write-through.

storeCustomer already updates caches; clearing immediately causes unnecessary cache misses.

-			try {
-				await botsStore.storeCustomer(interaction.user, normalizedEmail, existingCustomer.unthreadCustomerId);
-				// Proactively clear both discord and unthread keyed caches
-				await botsStore.clearCache('customer', interaction.user.id);
-				await botsStore.clearCache('customer', existingCustomer.unthreadCustomerId);
-			}
+			try {
+				await botsStore.storeCustomer(interaction.user, normalizedEmail, existingCustomer.unthreadCustomerId);
+				// No immediate invalidation needed; caches were refreshed by storeCustomer.
+			}

94-105: Precision cache invalidation (optional): avoid deleting non-existent keys.

clearCache('customer', ) deletes both discord and unthread keys; when passing a discordId or an unthreadId you’ll always delete one wrong key. If you need invalidation, prefer a targeted helper that accepts either discordId or unthreadCustomerId.

src/events/threadCreate.ts (1)

151-157: Reuse the fallback helper across modules (extract to utils).

To avoid drift with interactionCreate, consider moving generateDiscordUserEmail to a shared util and import it here and there.

src/services/webhook.ts (4)

175-176: Avoid logging full raw webhook payloads.

Recommend logging event/timestamp only; raw payloads may include sensitive data.

-	LogEngine.debug('Webhook received:', webhookReq.rawBody);
+	LogEngine.debug('Webhook received (headers + size)', {
+		'x-unthread-signature': req.headers['x-unthread-signature'],
+		length: typeof webhookReq.rawBody === 'string' ? webhookReq.rawBody.length : webhookReq.rawBody?.byteLength,
+		timestamp: new Date().toISOString(),
+	});

201-206: Log message contradicts behavior.

Message says “falling back to synchronous processing,” but we return 503. Adjust for clarity.

-	LogEngine.error('Queue processor not initialized - falling back to synchronous processing');
+	LogEngine.error('Queue processor not initialized - returning 503 (no sync fallback)');

238-254: Centralize retryability heuristic or reuse QueueProcessor’s logic.

String matching on error messages is brittle. If QueueProcessor exposes isRetryableError (or can), delegate to it; otherwise factor a shared helper.

-		const isInfrastructureFailure = errorMessage.includes('Redis') ||
-										errorMessage.includes('connection') ||
-										errorMessage.includes('timeout') ||
-										errorMessage.includes('ECONNREFUSED') ||
-										errorMessage.includes('network');
+		const isInfrastructureFailure =
+			(typeof queueProcessor?.isRetryableError === 'function'
+				? queueProcessor.isRetryableError(error as Error)
+				: /redis|connection|timeout|ECONNREFUSED|network/i.test(errorMessage));

133-147: Event priority list: type-safety and config-ability.

Consider typing against WebhookEventType and externalizing these arrays to config to avoid drift from Unthread’s schema.

Would you like me to generate a follow-up patch that pulls valid event names from src/types/unthread and gates unknown events to 'normal' with a warn log?

src/utils/threadUtils.ts (3)

116-148: Duplicate JSDoc block — consolidate to one canonical comment.

The second block restates the earlier docs; keep a single, up-to-date comment to reduce maintenance cost.

-/**
- * Fetches a Discord thread using an Unthread ticket ID with retry logic for race conditions
- * ...
- */

312-318: Global Discord client makes testing harder. Consider DI.

Passing the client as a parameter or injecting via a small context module improves testability and avoids global coupling.


238-240: Unreachable fallback comment.

“This should never happen…” branch is fine; consider converting to an assert utility so it’s caught in tests.

src/sdk/bots-brain/UnifiedStorage.ts (2)

477-485: Delete path: use allSettled to avoid bubbling cache errors.

 	async delete(key: string): Promise<void> {
-		await Promise.all([
+		await Promise.allSettled([
 			this.l1Memory.delete(key),
 			this.l2Redis.delete(key),
 			this.l3Postgres.delete(key),
 		]);

159-173: Use 'ready' event for node-redis readiness, not 'connect'.

‘connect’ fires before the client is ready to accept commands.

-			this.client.on('connect', () => {
+			this.client.on('ready', () => {
 				this.connected = true;
 				LogEngine.info('Redis L2 cache connected successfully');
 			});
src/index.ts (3)

80-92: Include database_pool in startup gate.

You build failedLayers from memory/redis/postgres only; if the PG pool itself is unhealthy, startup still passes. Recommend gating on database_pool too.

-    if (!health.memory) failedLayers.push('memory');
-    if (!health.redis) failedLayers.push('redis');
-    if (!health.postgres) failedLayers.push('postgres');
+    if (!health.memory) failedLayers.push('memory');
+    if (!health.redis) failedLayers.push('redis');
+    if (!health.postgres) failedLayers.push('postgres');
+    if ('database_pool' in health && !health.database_pool) failedLayers.push('database_pool');

158-169: Add tailored guidance when WEBHOOK_REDIS_URL is missing.

You log helpful hints for POSTGRES_URL/PLATFORM_REDIS_URL but not WEBHOOK_REDIS_URL. Add parity to reduce setup friction.

   if (!PLATFORM_REDIS_URL) {
     LogEngine.error('PLATFORM_REDIS_URL is required for L2 cache layer');
     LogEngine.error('Please provide a valid Redis connection URL (e.g., redis://localhost:6379)');
     process.exit(1);
   }
+
+  if (!process.env.WEBHOOK_REDIS_URL) {
+    LogEngine.error('WEBHOOK_REDIS_URL is required for webhook queue processing');
+    LogEngine.error('Please provide a valid Redis connection URL for the queue (e.g., redis://localhost:6379)');
+    process.exit(1);
+  }

Also applies to: 196-206


319-325: Harden raw body capture for undefined encoding.

Express may pass undefined; default to 'utf8' to avoid edge-case TypeErrors.

-verify: (req: express.Request, _res: express.Response, buf: Buffer, encoding: string) => {
+verify: (req: express.Request, _res: express.Response, buf: Buffer, encoding?: string) => {
   const webhookReq = req as WebhookRequest;
-  webhookReq.rawBody = buf.toString(encoding as BufferEncoding);
+  webhookReq.rawBody = buf.toString((encoding as BufferEncoding) ?? 'utf8');
},
src/sdk/bots-brain/BotsStore.ts (1)

287-321: Mapping mapper: sensible defaults and ISO normalization.

Fallback to now() when created_at missing is pragmatic; ensure upstream schema sets NOT NULL going forward.

Would you like me to open a follow-up to enforce NOT NULL on thread_ticket_mappings.created_at?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 547cc72 and 8c8f81c.

📒 Files selected for processing (9)
  • src/database/schema.sql (1 hunks)
  • src/events/interactionCreate.ts (3 hunks)
  • src/events/threadCreate.ts (2 hunks)
  • src/index.ts (6 hunks)
  • src/sdk/bots-brain/BotsStore.ts (1 hunks)
  • src/sdk/bots-brain/UnifiedStorage.ts (1 hunks)
  • src/services/QueueProcessor.ts (1 hunks)
  • src/services/webhook.ts (4 hunks)
  • src/utils/threadUtils.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/database/schema.sql
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.767Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#62
File: src/index.ts:339-342
Timestamp: 2025-09-05T15:49:33.268Z
Learning: In the wgtechlabs/unthread-discord-bot repository, for the v1.0.0 release, the user prefers to simplify webhook management endpoints by removing the retry endpoint entirely and making /webhook/health and /webhook/metrics show only general, non-sensitive information rather than detailed operational data.
📚 Learning: 2025-09-05T15:49:33.268Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#62
File: src/index.ts:339-342
Timestamp: 2025-09-05T15:49:33.268Z
Learning: In the wgtechlabs/unthread-discord-bot repository, for the v1.0.0 release, the user prefers to simplify webhook management endpoints by removing the retry endpoint entirely and making /webhook/health and /webhook/metrics show only general, non-sensitive information rather than detailed operational data.

Applied to files:

  • src/index.ts
  • src/services/webhook.ts
📚 Learning: 2025-08-28T11:18:55.767Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.767Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.

Applied to files:

  • src/index.ts
  • src/events/threadCreate.ts
📚 Learning: 2025-08-30T09:26:39.353Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#45
File: src/services/unthread.ts:184-193
Timestamp: 2025-08-30T09:26:39.353Z
Learning: In the wgtechlabs/unthread-discord-bot repository, the current custom Unthread service implementation in src/services/unthread.ts is planned to be replaced with an official SDK in the future, as mentioned by warengonzaga.

Applied to files:

  • src/index.ts
  • src/utils/threadUtils.ts
🧬 Code graph analysis (8)
src/services/QueueProcessor.ts (1)
src/types/unthread.ts (1)
  • WebhookPayload (180-187)
src/sdk/bots-brain/UnifiedStorage.ts (1)
src/config/logger.ts (1)
  • LogEngine (35-35)
src/events/interactionCreate.ts (1)
src/sdk/bots-brain/BotsStore.ts (1)
  • BotsStore (138-853)
src/index.ts (3)
src/sdk/bots-brain/BotsStore.ts (2)
  • BotsStore (138-853)
  • BotConfig (119-123)
src/services/webhook.ts (4)
  • initializeWebhookService (56-65)
  • webhookHandler (334-334)
  • webhookHealthCheck (335-335)
  • webhookMetrics (336-336)
src/types/discord.ts (1)
  • BotConfig (43-70)
src/events/threadCreate.ts (1)
src/utils/customerUtils.ts (1)
  • getOrCreateCustomer (89-120)
src/services/webhook.ts (2)
src/types/unthread.ts (1)
  • WebhookPayload (180-187)
src/services/QueueProcessor.ts (1)
  • QueueProcessor (94-611)
src/sdk/bots-brain/BotsStore.ts (2)
src/types/discord.ts (1)
  • ThreadTicketMapping (27-36)
src/sdk/bots-brain/UnifiedStorage.ts (1)
  • UnifiedStorage (380-558)
src/utils/threadUtils.ts (2)
src/types/discord.ts (1)
  • ThreadTicketMapping (27-36)
src/sdk/bots-brain/BotsStore.ts (2)
  • ExtendedThreadTicketMapping (107-114)
  • BotsStore (138-853)
🔇 Additional comments (13)
src/events/interactionCreate.ts (2)

197-201: Cleanup path looks good.

Clearing both mapping keys covers both directions; redundancy is acceptable here.


52-64: Resolved: Initialization Confirmed
Initialization sequence is validated, sir: BotsStore.initialize() runs at startup (src/index.ts:81) and QueueProcessor.initialize() in webhook service (src/services/webhook.ts:58).

src/events/threadCreate.ts (1)

20-34: Good call on RFC-safe fallback email.

Stable ID + .invalid is correct and non-deliverable. Nicely done.

src/services/webhook.ts (1)

56-65: Resolved: Webhook service initialization verified
Certainly, initializeWebhookService() is invoked at application startup in src/index.ts (line 97), so no further action is required.

src/index.ts (4)

27-29: Docs sync with storage stack looks good.

Accurately reflects POSTGRES_URL/PLATFORM_REDIS_URL/WEBHOOK_REDIS_URL. Proceed.


114-143: Health layering logic is solid.

Nicely degrades and returns structured layers + error. Carry on.


336-341: Endpoints align with v1.0.0 preference.

Retry removed; health/metrics retained for general info. Good.


370-381: Health endpoint behavior is appropriate.

503 on unhealthy, otherwise 200 with concise surface. Good.

src/sdk/bots-brain/BotsStore.ts (5)

248-279: Customer mapper: solid conversion and soft-delete handling.

Good camelCase mapping, warnings on missing created_at, and ISO output. Nicely done.


344-387: Upsert logic is correct; COALESCE prevents null-overwrite.

Write-through caching after DB commit is appropriate.


393-430: Cache-first get by Discord ID looks good.

Errors return null rather than throw—reasonable for reads.


531-568: Get mapping by thread ID: LGTM.

Cache warm paths and soft-delete filter are correct.


681-700: Health check: expose database_pool separately—nice.

Complements UnifiedStorage.ping() set. No changes requested.

Copilot AI and others added 16 commits September 12, 2025 17:22
- Update return type to provide detailed result with error messages
- Improve timeout error message with duration details
- Optimize Buffer handling in Blob creation
- Use bulk validation for better performance
- Enhance error tracking and user feedback

Co-authored-by: warengonzaga <[email protected]>
…-a8c9-6b01791a17f9

feat: implement Discord → Unthread image attachment upload system with Yarn v4 migration
…-8ab7-12da857882e5

🚀 feat: implement comprehensive CI/CD pipeline with Docker Hub and GitHub packages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants